Codecov Report

Attention: Patch coverage is 56.52174% with 10 lines in your changes missing coverage. Please review.

Project coverage is 65.28%. Comparing base (004d07b) to head (a286e29).

@@            Coverage Diff             @@
##             main    #4588      +/-   ##
==========================================
- Coverage   65.28%   65.28%   -0.01%     
==========================================
  Files         317      317              
  Lines       22277    22293      +16     
  Branches     3582     3583       +1     
==========================================
+ Hits        14544    14554      +10     
- Misses       5941     5946       +5     
- Partials     1792     1793       +1     

Hi @terryquigleysas this looks reasonable and is obviously a simple change code-wise but it may have lots of impact which I am not sure how to quantify. Before I approve this change, could you please add a short explanation as to why this change is required? Outside of the code maybe not being needed as mentioned in the issue you linked, what is the purpose of this change? How does removing the default BC provider enable the FIPs changes you are working towards and if we remove it how can we be certain that removing the default provider won't break anything else especially considering that some of the other settings given allow-read access by the policy file are BC related?

Thanks a bunch :)

Hi @terryquigleysas this looks reasonable and is obviously a simple change code-wise but it may have lots of impact which I am not sure how to quantify. Before I approve this change, could you please add a short explanation as to why this change is required? Outside of the code maybe not being needed as mentioned in the issue you linked, what is the purpose of this change? How does removing the default BC provider enable the FIPs changes you are working towards and if we remove it how can we be certain that removing the default provider won't break anything else especially considering that some of the other settings given allow-read access by the policy file are BC related?

Thanks a bunch :)

@scrawfor99 Sure, fair questions. As you say, the code changes appear minor but may have implications. This is part of a move to remove or replace any code that is either not FIPS-compliant or will not work if Bouncy Castle FIPS-approved jars are swapped in to replace their regular jars.

In this case the BouncyCastleProvider class does not exist in the FIPS libraries so this code would fail. In the POC code I brought in the FIPS libraries and was able to use the BouncyCastleFipsProvider equivalent. See 2.11...terryquigleysas:security:2.11#diff-e7ef66295cba81a49e4349781a2e9678d2b021a570e978bb4feb422b03d5d74aR334 . We cannot do this here as a) the 2 Bouncy Castle libraries cannot coexist on the same classpath b) using the FIPS libraries here would break other things, e.g. OpenSAML.

The cleanest way would be to remove this call entirely as has been suggested before. It was believed to be required for Blake2b and certificate handling but possibly for legacy reasons. I have tested the Blake2b code and the demo certificates against a local JDK 17, the bundled JDK 21 and run the Bulk Integration Tests. I too was concerned that there may be other test paths or scenarios where this should not be removed and that was where I would defer to the more in-depth knowledge you guys have of the code in order to be certain it will have no adverse side-effects.

There may be other approaches we can try but if we can safely remove this call it is by far the cleanest option.

If I remember correctly, the BouncyCastleProvider provides the default list of CIPHERS that are acceptable: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/ssl/util/SSLConfigConstants.java#L122-L234. Need to double check though.

I'm surprised that this change did not cause any tests to fail.

X-Posting this comment here as well.

@terryquigleysas Thinking about backwards compatibility, would there be an opportunity to pass a flag at build-time to create a distribution that is FIPS-compliant? Would it make sense to have multiple distributions? 1 with bouncycastle (BC) included and 1 without BC?

I was wondering if it made sense to add logic to check if the BouncyCastleProvider class was on the classpath. If it is on the classpath then can it add the bouncycastle provider? I've written some similar code before in netty: https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/BouncyCastlePemReader.java#L72-L94

Similar to the provider, is it possible to have different plugin-security.policy files depending on whether bouncycastle is provided or not?

X-Posting this comment here as well.

@terryquigleysas Thinking about backwards compatibility, would there be an opportunity to pass a flag at build-time to create a distribution that is FIPS-compliant? Would it make sense to have multiple distributions? 1 with bouncycastle (BC) included and 1 without BC?

I was wondering if it made sense to add logic to check if the BouncyCastleProvider class was on the classpath. If it is on the classpath then can it add the bouncycastle provider? I've written some similar code before in netty: https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/BouncyCastlePemReader.java#L72-L94

Similar to the provider, is it possible to have different plugin-security.policy files depending on whether bouncycastle is provided or not?

@cwperks Thank you for the code suggestion. That sounds like a sensible approach. While I have yet to see from my testing why the call is necessary I am wary that it's difficult in this case to say with 100% certainty what all the side effects will be. I will look to adding this more defensive approach.

A FIPS distribution is something that I have considered but my lack of familiarity with the OpenSearch build process has made it difficult for me to gauge how that would work and whether it would be feasible. There are several things to cover here, the first that spring to mind being:

• Would OpenSearch be open to having a separate FIPS distribution?
• What would that involve?
• Would this build also encompass all the plugins (e.g. opensearch-security/, opensearch-sql, opensearch-ml etc etc) that use Bouncy Castle libraries?
• Bouncy Castle FIPS libraries are currently only approved for Java 11 & 17. Would we ship a JDK 17 here? How does this tie in with the main branch now having a baseline of Java 21?

I used a slightly different policy file in the POC 2.11...terryquigleysas:security:2.11#diff-deaa12e077af166f8bbdea4f1784c2dc043f05292ac7313a857ab1542fe2405aR59 so yes, that is an option if required.

@peterzhuamazon Would you provide some insight into the OpenSearch build process? See comment above: #4588 (comment)

A FIPS distribution is something that I have considered but my lack of familiarity with the OpenSearch build process has made it difficult for me to gauge how that would work and whether it would be feasible. There are several things to cover here, the first that spring to mind being:

• Would OpenSearch be open to having a separate FIPS distribution?
• What would that involve?
• Would this build also encompass all the plugins (e.g. opensearch-security/, opensearch-sql, opensearch-ml etc etc) that use Bouncy Castle libraries?
• Bouncy Castle FIPS libraries are currently only approved for Java 11 & 17. Would we ship a JDK 17 here? How does this tie in with the main branch now having a baseline of Java 21?

@peterzhuamazon Would you provide some insight into the OpenSearch build process? See comment above: #4588 (comment)

A FIPS distribution is something that I have considered but my lack of familiarity with the OpenSearch build process has made it difficult for me to gauge how that would work and whether it would be feasible. There are several things to cover here, the first that spring to mind being:

• Would OpenSearch be open to having a separate FIPS distribution?
• What would that involve?
• Would this build also encompass all the plugins (e.g. opensearch-security/, opensearch-sql, opensearch-ml etc etc) that use Bouncy Castle libraries?
• Bouncy Castle FIPS libraries are currently only approved for Java 11 & 17. Would we ship a JDK 17 here? How does this tie in with the main branch now having a baseline of Java 21?

Hi, OpenSearch bundle rpm distribution can already run on FIPS enabled server (opensearch-project/opensearch-build#2099). As for whether we would have a separate FIPS distribution, I am not sure what is the issue of not able to use current distribution on FIPS enabled server.

If needed, you can open an issue in opensearch-build repo and we can discuss whether it is something we can probably add as official artifact, and in which types of distribution. This could involve a big amount of changes because all of our current logic as well as folder structures of our artifacts are under the assumption of platform/arch/dist leveling. Adding FIPS is definitely an extra factor that we are not sure how much impact involved until further research.

Thanks.

@cwperks I have pushed another commit based on your suggestion above. Thanks.

@terryquigleysas The new changes look good to me. Thanks!

How much effort do you think it would take to write a simple test?

i.e. If BCFIPS is provided on the classpath, ensure that the security plugin adds it as a security provider?

The other question I have is on the permissions in plugin-security.policy. Do you think it should include all permissions regardless of what is provided on the classpath?

The other question I have is on the permissions in plugin-security.policy. Do you think it should include all permissions regardless of what is provided on the classpath?

@cwperks As the other work progresses to support FIPS we should aim to separate FIPS/non-FIPS policies more. As it stands I felt including both here offered the ability to support adding both providers at little or no risk.

@terryquigleysas The new changes look good to me. Thanks!

How much effort do you think it would take to write a simple test?

i.e. If BCFIPS is provided on the classpath, ensure that the security plugin adds it as a security provider?

@cwperks I have been testing against a local deployment. I had to add another property to the policy file but it works as expected there.

I wouldn't be sure exactly which approach to use. Is there an example of something similar in the current test code? That is something else that may become clearer as the other work progresses.

I would actually expect us to set up the security file for the JDK/JVM to use Bouncy Castle FIPS provider but thought for completeness I could add the FIPS variant here too in much the same way as current done. I can remove the BCFIPS code and put it in as a separate PR is that is preferable. What do you think?

@terryquigleysas I think its possible to use an existing integration test and assert that Security.getProvider('BC') is non-null.

To test, I added a few lines to the end of InitializationIntegrationTest.testDefaultConfig to assert that the provider is not null. i.e. assertThat(Security.getProvider("BC"), notNullValue());

A similar check should work for BCFIPS if you provide the jar as a test dependency.

If you comment out the code to add the provider the test should fail.

@cwperks I have created #4605 to handle the initial refactor. This branch became unusable for me due to DOC and merge conflict issues. Apologies for any inconvenience.

I am not certain the BCFIPS changes I proposed are required. I will add those in a separate PR if they are ever needed.

Superseded by and referenced in #4605